Fix crash caused by unreachable SAL server by adding more SAL options#42
Fix crash caused by unreachable SAL server by adding more SAL options#42skuba31 wants to merge 1 commit intocherab:developmentfrom
Conversation
| def get_jet_sal(host: str = SAL_SERVER, **kwargs) -> SALClient: | ||
| """Creates a SALClient instance connected to `host` if provided. | ||
| By default uses env variable `JET_SAL_SERVER` if set, `https://sal.jetdata.eu` otherwise. | ||
| """ | ||
| return SALClient(host=host, **kwargs) | ||
|
|
||
|
|
||
| def __getattr__(name: str): |
There was a problem hiding this comment.
Unless Github's formatting is off, I think you're missing some indentation here: these functions should be class methods.
There was a problem hiding this comment.
No, the indentation is correct. I should have provided better explanation in the first description.
I wanted to create two ways for SAL creation, a function returning standard SAL class and a subclass that could potentially be expanded for other functionalities that jet.data.sal defines. I suppose one could be dropped in order to have one standardized way.
The custom getattr method should be on the module level to allow use of the current notation from cherab.jet.sal import sal introduced in #35 with the difference, that SAL server connection is checked only if this import is executed in code, not on package import itself.
This should ensure, that any scripts created with this notation remain working as long as SAL server is reachable.
The problem could be that custom getattr is available for python 3.7+. I am not sure what is the minimal python version cherab-jet aims to support.
There was a problem hiding this comment.
Ah, I see. That's a bit un-intuitive but makes sense. Essentially what you want is a lazy import, right? This could be done using importlib's LazyLoader (https://docs.python.org/3/library/importlib.html#implementing-lazy-imports): the intention may be a bit clearer to those reading the code.
There was a problem hiding this comment.
I suppose lazy import would work as well. I was trying to achieve something a bit different. The only thing in the module that needs special treatments is the sal attribute. Both the class and the function can be imported normally, as connection check happens on init or call.
2 participants
Fixes crash when importing cherab.jet while SAL server is unreachable #40
Adds new subclass and function that check sal reachability only on instantiation or call so that the import of package is unaffectd.
Also adds a custom getattr method of cherab.jet.sal module that allows to use former notation
from cherab.jet.sal import sal. Usage of this notation is discouraged as it results in the connection error being raised if SAL server is not reachable on import.